-
Notifications
You must be signed in to change notification settings - Fork 164
Add interface IDs directly to Typechain generated factories #1240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…sses - Replace generateInterfaceIds.js with addInterfaceIds.ts utility - Add interface IDs as static properties on Typechain factory classes - Export utility functions via src/utils.ts for programmatic use - Add ts-node dependency to run TypeScript build scripts - Remove separate interfaceIds.ts constants file
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the interface ID generation system by embedding ERC-165 interface IDs directly into Typechain-generated factory classes instead of maintaining a separate constants file.
- Replaced the ethers v5-based
generateInterfaceIds.jsscript with a TypeScript utility that adds interface metadata directly to factory files - Added incremental build support to only regenerate interface IDs when factory files are missing them
- Introduced new utility exports and package entry points for programmatic access to interface ID functions
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/interfaces/src/utils.ts | New utility export that re-exports interface ID functions for public API |
| packages/interfaces/src/interfaceIds.ts | Removed the auto-generated constants file containing interface ID mappings |
| packages/interfaces/src/index.ts | Removed export of the deleted interfaceIds module |
| packages/interfaces/scripts/utils/addInterfaceIds.ts | New TypeScript utility that adds interface IDs directly to factory classes |
| packages/interfaces/scripts/generateInterfaceIds.js | Removed the old ethers v5-based interface ID generation script |
| packages/interfaces/scripts/build.sh | Updated build process to use new TypeScript utility with incremental build check |
| packages/interfaces/package.json | Added new package exports and ts-node dependency for TypeScript build utilities |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Address Copilot review feedback for PR 1240: - Replace magic numbers with named constants for better readability - Replace fragile regex-based JSON parsing with Function constructor for more robust ABI parsing that handles complex TypeScript syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Add validation and improve code safety: - Add explicit justification comments for Function constructor usage - Add validation to ensure regex replacement succeeds before writing file - Fix ESLint warning by using const instead of let for content variable - Return false and log warning if metadata injection pattern not found
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1240 +/- ##
==========================================
+ Coverage 82.84% 84.05% +1.21%
==========================================
Files 47 42 -5
Lines 2093 2070 -23
Branches 620 615 -5
==========================================
+ Hits 1734 1740 +6
+ Misses 359 330 -29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…n fuzz test The test was failing when fuzzer generated inputs that left less than MIN_DELEGATION shares after undelegating but more than 0. Added constraint to ensure after undelegation either: - All tokens are undelegated (remaining = 0), which is valid, OR - At least MIN_DELEGATION remains, which is the minimum required The root cause: The contract correctly enforces that delegators must either fully exit (0 shares) or maintain at least MIN_DELEGATION shares. The test was missing this constraint, allowing the fuzzer to generate invalid intermediate amounts like 0.95e18 shares remaining. Fixes the intermittent CI failure on fuzzing seed 0xd5262092.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Extract indentation from existing code instead of hardcoding 2 spaces - Track and report processing failures separately from skipped files - Return null from addInterfaceIdToFactory on errors instead of false - Throw error if any files failed processing to fail the build - Change warning to error for pattern match failures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tmigone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, i like this approach!
Instead of generating a separate constants file, interface IDs are now added directly as static properties on Typechain-generated factory classes.
Build process
generateInterfaceIds.js(ethers v5) toaddInterfaceIds.ts(ethers v6)ts-nodedependency (from catalog) to run TypeScript build utilitiesAPI changes
src/interfaceIds.ts- no longer generating a separate constants filesrc/utils.ts- exports utility functions (calculateInterfaceId,addInterfaceIdToFactory,addInterfaceIds) for programmatic useinterfaceIdandinterfaceNameas static propertiesExample usage